Update for Swift Concurrency in Swift 6#29
Conversation
There was a problem hiding this comment.
Code Review — Swift 6 Concurrency Migration
Summary: A well-executed strict-concurrency migration — the two managers collapse into a single @BluetoothActor, Peripheral/AdvertisementData become pure Sendable value snapshots, Combine is replaced by per-subscriber AsyncStream broadcasters with replay-on-subscribe, and the lazy-authorization + CBCentralManagerFactory(forceMock:) constraints are preserved. The architecture is sound; the items below are reachable correctness/lifecycle edges, not structural flaws.
🔴 Must-fix
-
BluetoothActor.peripheralDiscoveriesStream()— unbounded buffer. It uses the default (.unbounded) buffering policy whilestateStream()anddiscoveredPeripheralsStream()use.bufferingNewest(1). BLE advertisements arrive at high frequency, so a slow or abandonedfor awaitconsumer grows memory without bound. UseAsyncStream(bufferingPolicy: .bufferingNewest(N))or explicitly document the unbounded tradeoff. (The docstring covers no-replay, but not buffering.) -
BluetoothActor.handlePeripheralDiscovered— name-based identity collapses distinct devices. Identity is keyed onname ?? localName ?? identifier.uuidString, andPeripheralisHashable/==onidonly. Two physical devices advertising the same name collapse into onediscoveredPeripheralsentry, andcbPeripherals[resolvedId] = cbPeripheraloverwrites the live reference — soconnect(to:)can target whichever was seen last. ThecbIdentifierfallback branch only rescues the same device with a changed name, not this collision. Prefer keying the live registry/dedup on the stablecbPeripheral.identifier. (Partly acknowledged by theFR-8.5TODO — at minimum surface it as a documented limitation.) -
ReliaBLEManager.init— fire-and-forget init ordering race.Task { await BluetoothActor.shared.initialize(...) }isn't ordered before a caller's immediateawait startScanning(); actors give no cross-Task FIFO guarantee, so an early scan can no-op againstcentralManager == nil. The inline comment acknowledges this. Consider an actor-sideensureInitialized()that scanning/authorize funnel through (preserving lazy permission) rather than relying on the guard's silent no-op.
🟡 Suggestions
-
authorize()/authorizeBluetooth()returns before resolution. For.notDeterminedit callssetupCentralManager()and returns immediately — the user can still deny the system prompt aftertry await authorizeBluetooth()has succeeded. Either suspend until the authorization callback resolves, or rename/document it as "request authorization" and point callers to thestatestream for the result. -
BluetoothDelegateShim— delegate ordering. Each callback spawns an independentTask { @BluetoothActor in … }. Handlers run atomically (no internal awaits), but adidDiscovertask can be scheduled after aresetting→invalidatePeripherals()task, repopulatingcbPeripheralspost-invalidation. Low probability; consider funneling delegate events through one ordered sequence to guarantee CB-queue order. -
LoggingService.enabled— mutable var on aSendableclass. Forwards straight toWillow.Logger.enabled. Safe only if Willow synchronizes that property internally — worth confirming; otherwise make enablement configure-once. -
Tests. Good
Sendable/replay coverage, but the highest-risk paths above are untested. Add: init→immediate-scan ordering, stream cancellation pruning continuations, same-name duplicate peripherals, andauthorizeBluetooth()from.notDeterminedthrough denial. -
Breaking API. Combine→AsyncStream, several ops now
async,Peripheralnow a value snapshot, manager split removed — source-breaking, so release as a major version and confirm README/DocC examples match.
❓ Questions
- Is collapsing same-named devices an accepted interim behavior pending
FR-8.5, or shouldcbIdentifierbecome the dedup key now? - Should
connect(to:)with anilcentral manager throw (it currently logs and silently returns, unlike thenotFoundthrow path)?
Authored-By: Claude Opus 4.8 [email protected]
Must-fix
Suggestions
Questions
|
…elegate callbacks, suspend authorize - peripheralDiscoveriesStream: bound buffer to .bufferingNewest(10_000) (~10MB cap) instead of unbounded, so a stalled subscriber drops oldest events rather than growing memory without limit. - ensureInitialized(log:): idempotent actor setup that every public ReliaBLEManager entry point funnels through, closing the init-vs-immediate-call race. It also (re)creates the central manager on any entry when already .allowedAlways, so operations work after out-of-band authorization while preserving the lazy-permission contract. - Ordered delegate handling: BluetoothDelegateShim yields each callback into a single AsyncStream<DelegateEvent> drained by one consumer task, preserving CoreBluetooth's serial callback order (replaces per-callback Tasks that could reorder). - Suspending authorize: .notDetermined now suspends until centralManagerDidUpdateState resolves the decision; cancellation is wired via withTaskCancellationHandler in the nonisolated ReliaBLEManager facade, keeping the actor method clear of a construct the Swift 6.1 region-isolation checker cannot analyze. - connect(to:) throws PeripheralError.bluetoothUnavailable (new case; enum now Equatable) when no central manager exists, instead of silently returning. - Expanded the FR-8.5 TODO documenting the same-name peripheral identity collision. - Tests updated for the new behaviors (cancellable authorize, broadened connect error). Co-Authored-By: Claude Opus 4.8 <[email protected]>
…e-object-protocol-to-the-observable-macro Migrating from the ObservableObject protocol to the @observable macro
Pulled in updated Willow and updated for Swift 6
#13) Moves CBCentralManager, the discovered-peripherals list, and Combine subjects into a new @globalActor BluetoothActor, with a BluetoothDelegateShim bridging CBCentralManagerDelegate callbacks into actor-isolated handlers. BluetoothManager and ReliaBLEManager become thin async forwarders, PeripheralManager is folded into the actor, and docs/demo call sites are updated for the new async API. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- Restore stored peripheralIdentifier: seed in init, refresh in update, don't clear in invalidateCBPeripheral. Fixes refreshPeripherals after invalidation (Rec 1, Copilot comments #1/#2/#3). - Restore dedicated serial DispatchQueue for CBCentralManager instead of queue: nil, keeping callbacks off the main thread (Rec 2, comment #9). - Adopt plan-prescribed @BluetoothActor global-actor annotation in BluetoothDelegateShim Task hops (Rec 3, comment #6). - Remove redundant nonisolated on hash(into:) and fix CBPeriphal typo in == doc comment (Rec 4, comments #4 partial / #5). Co-Authored-By: Claude Opus 4.8 <[email protected]>
…6-safe Introduce BluetoothActor to serialize CoreBluetooth state (Step 1 of 5)
Convert `Peripheral` from a reference-wrapping type into an immutable, `Sendable` value snapshot that carries no `CBPeripheral` reference. The live `CBPeripheral` is now owned exclusively by `BluetoothActor` in an `id`-keyed registry that never escapes the actor; operations forward the snapshot's `id` and the actor resolves the live reference, throwing `PeripheralError.notFound` when a snapshot has gone stale. - Add `AdvertisementData` to expose typed advertisement fields as values - Add `PeripheralError` for stale/unresolvable snapshot lookups - Add `CBUUID+Sendable` retroactive conformance - Let apps pre-register a known peripheral via `Peripheral(id:)` - Update `BluetoothActor` discovery to snapshot values and retain the non-`Sendable` CoreBluetooth payload only inside the actor - Update DocC and tests for the new value-type API
I don't want this to be a tempting escape hatch later
I reviewed and all seem reasonable but will need validated with a build and test. Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Make Peripheral a Sendable value type (Step 2 of 5 for #10)
…f 5, #10) Replace the three AnyPublisher surfaces on ReliaBLEManager (state, peripheralDiscoveries, discoveredPeripherals) with per-subscription AsyncStreams fed by an in-BluetoothActor broadcaster, and remove Combine from Sources/ReliaBLE and the Demo. - BluetoothActor: three [UUID: AsyncStream.Continuation] dictionaries replace the Combine subjects/publishers; nonisolated stream factories mint a fresh stream per call and register on an actor hop. state/discoveredPeripherals replay their latest value (.bufferingNewest(1)); peripheralDiscoveries does not replay (unbounded). currentBluetoothState retained as the sync currentState backing + state replay source. - BluetoothManager/ReliaBLEManager: retype the three properties to AsyncStream. - Demo: consume via .task { for await } loops feeding @mainactor handlers; drop cancellables/setupSubscriptions. - Tests: add broadcaster coverage (multi-subscriber replay, fan-out, no-replay). - DocC: replace .sink examples with for await; document streaming semantics. Co-Authored-By: Claude Opus 4.8 <[email protected]>
dropping the indirection layer now that the actor work is done directly. Move BluetoothState/AuthorizationError types and the actor-isolated state snapshot onto ReliaBLEManager, delete BluetoothManager.swift, and update docs/tests to match the new public surface.
Replace Combine event surfaces with AsyncStream broadcaster (Step 3 of 5, #10)
Mark ReliaBLEConfig Sendable, and add DocC concurrency guide. Make Swift 6 language mode and complete strict concurrency explicit in Package.swift for the ReliaBLE and ReliaBLEMock targets rather than relying on the swift-tools-version default, mark ReliaBLEConfig as Sendable, and document the actor isolation contract in a new DocC Concurrency article. Also record the planning and critique notes for this step of the Swift 6 migration.
Collapse BluetoothManager into ReliaBLEManager (Step 4 of 5, #10)
Move all SwiftData persistence in the Central tab off the main thread by introducing `DeviceStoreActor`, a `ModelActor` created off-main via `create(container:)`. `CentralView` now wires reliaBLE streams and the store through a single `.task`/`withTaskGroup`, `CentralViewModel` keeps only UI state and BLE actions, and `Demo/AGENTS.md` documents the new pattern. Adds the accompanying planning doc.
Added Swift 6 strict concurrency checking (Step 5 of 5, #10)
Use Task.detached in create(container:) so ModelActor construction is guaranteed off-main even when called from SwiftUI .task. Use peripheral.lastSeen consistently on new Device inserts. Fix AGENTS.md to describe manual ModelActor conformance, not the @Modelactor macro.
Demo: route SwiftData writes through background DeviceStoreActor (#20)
…elegate callbacks, suspend authorize - peripheralDiscoveriesStream: bound buffer to .bufferingNewest(10_000) (~10MB cap) instead of unbounded, so a stalled subscriber drops oldest events rather than growing memory without limit. - ensureInitialized(log:): idempotent actor setup that every public ReliaBLEManager entry point funnels through, closing the init-vs-immediate-call race. It also (re)creates the central manager on any entry when already .allowedAlways, so operations work after out-of-band authorization while preserving the lazy-permission contract. - Ordered delegate handling: BluetoothDelegateShim yields each callback into a single AsyncStream<DelegateEvent> drained by one consumer task, preserving CoreBluetooth's serial callback order (replaces per-callback Tasks that could reorder). - Suspending authorize: .notDetermined now suspends until centralManagerDidUpdateState resolves the decision; cancellation is wired via withTaskCancellationHandler in the nonisolated ReliaBLEManager facade, keeping the actor method clear of a construct the Swift 6.1 region-isolation checker cannot analyze. - connect(to:) throws PeripheralError.bluetoothUnavailable (new case; enum now Equatable) when no central manager exists, instead of silently returning. - Expanded the FR-8.5 TODO documenting the same-name peripheral identity collision. - Tests updated for the new behaviors (cancellable authorize, broadened connect error). Co-Authored-By: Claude Opus 4.8 <[email protected]>
…overies - GettingStarted: authorizeBluetooth() now suspends until the user responds and returns only when authorized (and is cancellable) — document the new behavior in place of the old "no-op" note. - GettingStarted: the connect(to:) example now also handles PeripheralError.bluetoothUnavailable. - GettingStarted & Concurrency: note that the peripheralDiscoveries feed is bounded (drops the oldest pending advertisements under backpressure). Co-Authored-By: Claude Opus 4.8 <[email protected]>
efd72f2 to
daa5601
Compare
Route store writes through Task.detached so MainActor-inherited tasks do not run SwiftData mutations on the main thread. Delete by fetching in the actor's ModelContext instead of model(for:) with IDs from @query.
Previously, PeripheralManager unconditionally instantiated CBPeripheralManager in init(), causing the system Bluetooth permission prompt on a fresh app launch. Move creation into startAdvertising() so the prompt only appears when the user explicitly chooses to use the peripheral advertising feature. This prevents the demo from appearing to trigger authorization automatically (via the library or otherwise). Also update the Start button enable logic to allow activation while the manager is still uninitialized.
6b50aed to
e40bb7d
Compare
Summary
Modernizes ReliaBLE for Swift 6 with complete concurrency checking. This is the umbrella PR for issue #10, landing the 5-step migration:
BluetoothActorto serialize all CoreBluetooth state (replaces ad-hoc queue hopping).PeripheralaSendablevalue type (noCBPeripheralreference escapes the actor).AsyncStreambroadcasters (replay-on-subscribe for state & discovered peripherals).BluetoothManager+PeripheralManagerintoReliaBLEManager(anonisolated,Sendablefaçade forwarding to the actor).Package.swift.New supporting types:
AdvertisementData,PeripheralError,CBUUID+Sendable.Architecture notes
CBCentralManager, continuations, discovered peripherals, liveCBPeripheralregistry) is owned exclusively by@BluetoothActor.BluetoothDelegateShim..allowedAlwaysor whenauthorizeBluetooth()is called, so the integrating app controls the permission prompt.CBCentralManagerFactory.instance(..., forceMock: true)is retained (load-bearing for theReliaBLEMocktest target).Breaking changes
Source-breaking for consumers: Combine publishers →
AsyncStream, several operations are nowasync, the manager split is removed, andPeripheralis now a value snapshot. Should ship as a major version.🤖 Generated with Claude Code